Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(deps): update reqwest #769

Closed
wants to merge 2 commits into from
Closed

Conversation

flavio
Copy link
Contributor

@flavio flavio commented May 22, 2024

Issue #, if available:

The current version of tough depends on an older version of reqwest which in cause a old version of hyper to be consumed.

This hyper upgrade is massive, having a unified version of hyper inside of projects consuming the tough crate is beneficial. For example, hyper testing frameworks can work only with one version of the library at a time.

Description of changes:

Update to latest version of reqwest, v0.12. This version depends on hyper v1, while previous versions (like v0.11) rely on hyper v0.14.

@flavio
Copy link
Contributor Author

flavio commented May 22, 2024

I'll look into the failed unit tests, sorry... I didn't spot them locally

@webern
Copy link
Contributor

webern commented May 22, 2024

I'll look into the failed unit tests, sorry... I didn't spot them locally

It's an unfortunate lint that we have using Cargo Deny. The lint causes an error when two major versions of a dependency exist in Cargo.lock. To fix the lint, the list of ignored dependencies has to be updated in deny.toml in the bans skip section

skip = [

make check-licenses will run this locally

tough/Makefile

Line 16 in eb5e25e

cargo deny --all-features check --disable-fetch licenses bans sources

@flavio
Copy link
Contributor Author

flavio commented May 23, 2024

@webern I've updated cargo.deny. Unfortunately the list of duplicated crates is quite bit. Quite some of them come from crates related with aws-smithy. Maybe you have influence over them?

@flavio
Copy link
Contributor Author

flavio commented May 23, 2024

@webern sorry, this should be fixed now

@webern
Copy link
Contributor

webern commented May 23, 2024

sorry, this should be fixed now

Can you also turn off auto-formatting of TOML files and restore them so that the PR has the minimum possible diff? We prefer to avoid auto-formatting churn in our git history.

Thank you!

@flavio
Copy link
Contributor Author

flavio commented May 23, 2024

@webern sorry about that, I fixed it.

Moreover, make ci completes successfully on my computer now.

@webern
Copy link
Contributor

webern commented May 24, 2024

Looks good, thank you for dealing with our deny.toml 🙄

@flavio
Copy link
Contributor Author

flavio commented May 30, 2024

Sorry to bother you, do you have any ETA about when you plan to merge this PR and even tag a new release of the crate? 🙏

@jpculp
Copy link
Contributor

jpculp commented Jul 2, 2024

Sorry for the radio silence here. I was concerned about the number of duplicated crates that were being pulled in. I'm going to poke at this early next week and see how things look once we bump smithy and aws-sdk-rust. Do you mind if I add your commit to that larger dependency update PR, or would you rather rebase this PR on top of it?

@flavio
Copy link
Contributor Author

flavio commented Jul 3, 2024

Thanks for looking into that! I'm fine with you adding this commit to a larger dependency PR

@jpculp
Copy link
Contributor

jpculp commented Jul 9, 2024

After bumping some things in #781, I still couldn't seem to get out of the situation where we pull two versions of hyper. From what I can tell, support for v1 is only available in aws-smithy-experimental. If we wanted to stick with the stable release then anyone using tough and aws-smithy would end up having the two versions of hyper you're looking to avoid. Just so I can understand better, can you elaborate on your use-case for hyper v1?

@flavio
Copy link
Contributor Author

flavio commented Jul 10, 2024

I need tough to consume latest version of reqwest to address this issue.

tough/Cargo.toml Outdated
@@ -22,7 +22,7 @@ log = "0.4"
olpc-cjson = { version = "0.1", path = "../olpc-cjson" }
pem = "3"
percent-encoding = "2"
reqwest = { version = "0.11", optional = true, default-features = false, features = ["stream"] }
reqwest = { version = "0.12", optional = true, default-features = false, features = ["stream"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reqwest = { version = "0.12", optional = true, default-features = false, features = ["stream"] }
reqwest = { version = "0.11", optional = true, default-features = false, features = ["stream"] }
reqwest-hyper-v1 = { package = "reqwest", version = "0.12", optional = true, default-features = false, features = ["stream"] }

I wonder if we could do this and separate them by feature. Crates stuck on the old hyper would continue to use http and those on the new could use a http-hyper-v1. Curious if @bcressey or @cbgbt have thoughts around this as well.

@bcressey
Copy link
Contributor

From what I can tell, support for v1 is only available in aws-smithy-experimental.

From the crate description:

This crate allows customers to use Hyper 1.0. A valuable consequence of this is access to aws-lc-rs and its FIPS compliant crypto. This is available behind the crypto-aws-lc-fips feature. Note: FIPS support has somewhat complex build requirements, namely CMake and Go

This will be required soon for FIPS support in updog, so I would like to start moving in that direction.

@flavio
Copy link
Contributor Author

flavio commented Aug 30, 2024

Hello, is there any update on bumping the hyper/reqwest?

This is blocking the release of sigstore-rs crate 😢

flavio added a commit to flavio/sigstore-rs that referenced this pull request Sep 4, 2024
Sigstore's TUF repository format changed in way that breaks the
tough crate.

This commit updates to a forked release of the tough crate that includes
fixes for these issues:
* awslabs/tough#778 - the fix for the TUF repo change
* awslabs/tough#769 - another long standing
  issue

Signed-off-by: Flavio Castelli <[email protected]>
@jpculp
Copy link
Contributor

jpculp commented Sep 4, 2024

@flavio, sorry about the delay on this. I'm still trying to get some answers on aws-smithy-experimental. If it turns out that it is still too early to take advantage of it, what are your thoughts on the feature flag workaround above?

Update to latest version of reqwest, v0.12. This version depends on
hyper v1, while previous versions (like v0.11) rely on hyper v0.14.

This hyper upgrade is massive, having a unified version of hyper inside
of projects consuming the tough crate is beneficial. For example, hyper
testing frameworks can work only with one version of the library at a
time.

`cargo deny` had to be configured to ignore some duplicated crates.
The reqwest update is massive, is the pinnacle of a major update of hyper.
A lot of hyper dependencies received major bumps too.

Some dependencies of tough (including development only ones), have not
been updated to consume these new crates.

Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Flavio Castelli <[email protected]>
@flavio
Copy link
Contributor Author

flavio commented Sep 5, 2024

@jpculp I think using the feature flag is the only way to go, and it works (I tested it).

I've force pushed the following changes:

  • Rebase against development branch
  • Initial patch
  • Introduce feature flag

Once you're happy about the changes I'll squash the 2 commits into a single one. I just wanted to make easy to understand the amount of changes requested by the feature flag.

@flavio
Copy link
Contributor Author

flavio commented Sep 10, 2024

Fantastic news folks, because of a happy coincidence I finally understood why sigstore main branch wasn't working with tough anymore.
I wouldn't have been able to understand what was going on without 1) the automated PR triggering my curiosity 2) the proposal made here to include different versions of reqwest

About this PR

The Sigstore project doesn't need any bump of reqwest by tough. We can solve the issue all on our own (more details below).

Having tough bump reqwest would still be nice, but this commit is definitely not needed.

There might still be value inside of the 1st commit, because of the updates to cargo.deny. However I think you probably won't need them once the whole "AWS ecosystem" moves to the latest version of reqwest.

Feel free to close this PR.

The root cause and the fix

It can be found here

@jpculp
Copy link
Contributor

jpculp commented Sep 10, 2024

@flavio, that's fantastic news! On our side we're actually working on the code to jump to aws-smithy-experimental for full hyper v1 support. After talking to some folks on our end, we found it might not be as heavy of a lift as we were expecting. Thank you for your patience in all of this!

@jpculp
Copy link
Contributor

jpculp commented Sep 19, 2024

It's still in progress, but we're working on the aws-smithy-experimental switch in #826.

@jpculp
Copy link
Contributor

jpculp commented Sep 27, 2024

Good news. We merged #826 and should be ready for reqwest 0.12 now. Are you able to undo the feature toggle and rebase, or would you rather we just take care of it from here?

@ginglis13
Copy link
Contributor

Updated reqwest to 0.12 in #828 - thanks for paving the way @flavio !

@ginglis13 ginglis13 closed this Oct 1, 2024
@flavio
Copy link
Contributor Author

flavio commented Oct 2, 2024

@jpculp, @ginglis13 thanks for having handled that. I'm sorry, but the last days have been pretty busy for me

@flavio flavio deleted the update-reqwest branch October 2, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants